Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial API proposal #9

Closed

Conversation

yamamoto0104
Copy link
Contributor

What type of PR is this?

  • enhancement/feature

What this PR does / why we need it:

This is the initial proposal from KDDI for the Number Recycling API

Which issue(s) this PR fixes:

Fixes #8

Special notes for reviewers:

None

Changelog input

 release-note
- Initial API proposal

Additional documentation

None

@yamamoto0104 yamamoto0104 requested a review from a team October 8, 2024 12:10
@eric-murray
Copy link
Contributor

I'm converting this to draft until the linting rules (PR #7) have been merged into the repository

@eric-murray eric-murray marked this pull request as draft October 8, 2024 12:16
@yamamoto0104 yamamoto0104 marked this pull request as ready for review October 8, 2024 12:44
@yamamoto0104 yamamoto0104 marked this pull request as draft October 8, 2024 12:57
@yamamoto0104 yamamoto0104 marked this pull request as ready for review October 18, 2024 02:44
@yamamoto0104
Copy link
Contributor Author

The linting rules (PR#7) have been merged into the repository. Thus, I converted this to Open.

CreateCheckNumRecycling:
type: object
required:
- phoneNumber
Copy link
Contributor

@eric-murray eric-murray Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have made phoneNumber required, but the network subscriber will also be identified by a 3-legged access token (this will be the usual method of calling this API according to CAMARA guidelines). Can you explain the API logic when both a 3-legged token and explicit phoneNumber are received?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-murray
Because we assume that this API uses 2-legged access token.
As the user story mentioned, ASP checks whether there has been a change in the user associated with the phone number after the specified date so that API can ensure that a phone number is correctly linked to an user and prevent mis-delivery before sending SMS messages to an wrong user (if there has been a change).
If we apply 3-legged access token, we cannot use this API. Because ASP may authenticate the wrong user and potentially may ask for the wrong user's consent for the API access with 3-legged access token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yamamoto0104
This is not compliant with ICM requirements, which state:

... in cases where personal user data is processed by the API, and users can exercise their rights through mechanisms such as opt-in and/or opt-out, the use of 3-legged access tokens becomes mandatory

In Europe, for example, end users have the right to opt-out from such anti-fraud APIs processing their data, and hence 3-legged tokens are mandatory. We cannot say to an end user that their legal rights no longer apply just because they might be a fraudster.

Exactly the same situation arises with the SIM Swap API, which does (indeed, must) allow 3-legged tokens. The implementations I am aware of only support CIBA, which does not require any interaction with the end user device itself. The same would apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @eric-murray
Thank for your comment.

As the description in SIM Swap API yaml file said, 2-legged access tokens can be applicable as one of the options.
If my understanding is incorrect, please let me know.

Restrictions for tokens without an associated authenticated phone number:

  • For scenarios which do not have a phone number associated to the token during the authentication flow, e.g. 2-legged access tokens, the phoneNumber field MUST be provided in the API request. This ensures that the phone number is explicit and valid for each API call made with these tokens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yamamoto0104

Indeed, if the SIM Swap API is called with a 2-legged access token, then phoneNumber must be provided. But if it is called with a 3-legged token, then a phoneNumber MUST NOT be provided. So the phoneNumber is not a required parameter. This can be seen from the SIM Swap CreateCheckSimSwap schema.

In your proposed API definition, phoneNumber is always a required parameter. So the API consumer would be forced to provide it, irrespective of whether at 2- or 3-legged token was being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eric-murray
Thank you for your kind response.
We changed phoneNumber as optional and added the phone number description based on SIM-SWAP yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eric-murray

We updated the phone number description based on the Appendix of API design guidelines. We also added 422 error code based on the Appendix.

Appendix A: info.description template for device identification from access token

Copy link
Contributor

@eric-murray eric-murray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing the mandatory ICM template in the info section.

description: Product documentation at Camara
url: https://github.com/camaraproject/NumberRecycling
servers:
- url: '{apiRoot}/number-recycling/v0.1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- url: '{apiRoot}/number-recycling/v0.1'
- url: '{apiRoot}/number-recycling/vwip'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, The version was changed.

description: Correlation id for the different services
schema:
type: string
schemas:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common PhoneNumber schema has been defined by Commonalities, so we should use it here

Suggested change
schemas:
schemas:
PhoneNumber:
description: A public identifier addressing a telephone subscription. In mobile networks it corresponds to the MSISDN (Mobile Station International Subscriber Directory Number). In order to be globally unique it has to be formatted in international format, according to E.164 standard, prefixed with '+'.
type: string
pattern: '^\+[1-9][0-9]{4,14}$'
example: "+123456789"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out. We added the common PhoneNumber schema.

Comment on lines 115 to 119
phoneNumber:
type: string
pattern: '^\+[1-9][0-9]{4,14}$'
example: '+346661113334'
description: A public identifier addressing a telephone subscription. In mobile networks it corresponds to the MSISDN (Mobile Station International Subscriber Directory Number). In order to be globally unique it has to be formatted in international format, according to E.164 standard, prefixed with '+'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference separate schema

Suggested change
phoneNumber:
type: string
pattern: '^\+[1-9][0-9]{4,14}$'
example: '+346661113334'
description: A public identifier addressing a telephone subscription. In mobile networks it corresponds to the MSISDN (Mobile Station International Subscriber Directory Number). In order to be globally unique it has to be formatted in international format, according to E.164 standard, prefixed with '+'.
phoneNumber:
$ref: '#/components/schemas/PhoneNumber'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Reference separate schema is added.

@yamamoto0104 yamamoto0104 dismissed eric-murray’s stale review October 29, 2024 09:47

The merge-base changed after approval.

eric-murray
eric-murray previously approved these changes Oct 29, 2024
@yamamoto0104 yamamoto0104 dismissed stale reviews from grgpapadopoulos and eric-murray October 30, 2024 08:44

The merge-base changed after approval.

eric-murray
eric-murray previously approved these changes Oct 30, 2024
eric-murray
eric-murray previously approved these changes Oct 30, 2024
@yamamoto0104 yamamoto0104 dismissed eric-murray’s stale review October 30, 2024 10:15

The merge-base changed after approval.

@eric-murray
Copy link
Contributor

@yamamoto0104
This PR appears to be "stuck", as any approvals are being automatically dismissed by github.

I'd suggest either:

  • Re-sync your fork with the CAMARA repository to ensure the base is correct; or
  • Close this PR and open a new one using the currently proposed YAML

Alternatively, @hdamker may be able to override whatever github settings are automatically dismissing approvals.

@eric-murray
Copy link
Contributor

@yamamoto0104
Your own branch does not include the user story, so this PR effectively "deletes" that. You need to copy it in to your branch from the CAMARA repository.

@yamamoto0104
Copy link
Contributor Author

@eric-murray
Thank you for your feedback. I'll check the status with Toshi and fix this tomorrow.

@hdamker
Copy link
Contributor

hdamker commented Oct 30, 2024

Your own branch does not include the user story, so this PR effectively "deletes" that. You need to copy it in to your branch from the CAMARA repository.

It was actually explicitly deleted within commit 3c20232 from the branch ("Delete User story.md for error.").

Beyond that the PR now looks good ... guess you don't need to bypass the branch protection.

@yamamoto0104
Copy link
Contributor Author

Hello @hdamker, @eric-murray
As Eric mentioned, I need to close this PR and open a new one using the currently proposed YAML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial proposal for Number Recycling API definition
4 participants